[#232] Durable rate limiting for view counts#237
Conversation
Replace volatile in-memory IP rate limiter with a durable session-based rate limit that queries page_views count per session per hour. Survives serverless cold starts and doesn't rely on x-forwarded-for. Fixes #232 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The durable rate-limit approach is reasonable, but the new count query currently ignores Supabase errors. If that lookup fails, the endpoint silently skips throttling for the request instead of surfacing a server error.
Findings
- [medium] The
page_viewscount query used for rate limiting does not check itserrorresult, so database failures disable the limiter instead of returning a 500.- File:
src/app/api/views/route.ts:79 - Suggestion: capture
errorfrom theselect(..., { count: "exact", head: true })call and returnerror(Database error: ${dbError.message}, 500)before evaluatingrecentCount.
- File:
Decision
Requesting changes because the new durable limiter currently fails open when the backing count query errors.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The DB-error handling fix landed, but the durable limiter still introduces a behavior regression and a schema-performance gap. As written, it throttles normal browsing across unrelated stories and backs the new query with no useful index.
Findings
- [medium] The new rate-limit query counts all
page_viewsfor asession_idglobally, which is broader than the previous per-storyline/per-page limiter and will 429 normal browsing after 10 distinct views in an hour.- File:
src/app/api/views/route.ts:79 - Suggestion: scope the count to at least
.eq("storyline_id", storylineId)to preserve prior behavior, and includeplot_indexif you want to stay aligned with the original per-page key. If the global cap is intentional, it needs to be explicitly documented and the threshold likely needs to be much higher.
- File:
- [medium] The new count query filters on
(session_id, viewed_at)but the existing indexes are onstoryline_id,(storyline_id, plot_index),(storyline_id, plot_index, session_id, viewed_at), andcontract_address. None of those support the new query shape efficiently, so this will degrade into table scans aspage_viewsgrows.- File:
src/app/api/views/route.ts:79 - File:
supabase/migrations/00007_page_views.sql:14 - Suggestion: add a migration with an index tailored to the new lookup, such as
CREATE INDEX idx_page_views_session_rate ON page_views(session_id, viewed_at);or a more selective composite that matches the final scoped query.
- File:
Decision
Requesting changes because the durable limiter still regresses request scope and lacks the DB index needed for the new query pattern.
- Add storyline_id filter to rate limit query (prevents blocking users who browse many storylines) - Add index on (session_id, viewed_at) for efficient rate limit counts Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
Re-review complete. The durable limiter now scopes counts per session_id and storyline_id, and the new migration adds the missing index for the session-based lookup.
Findings
- None. The follow-up addresses the previously requested scope and performance issues.
Decision
Approving because the updated rate-limit query now preserves the intended behavior more closely and is backed by a matching database index. CI was still pending at the time of review.
2 participants
Summary
page_viewsentries persession_idin the last hour — max 10 per sessionx-forwarded-fordependency (not spoofable)Test plan
Fixes #232
🤖 Generated with Claude Code